Skip to content

Various fixes against the v25 branch.#13

Open
oliverbestmann wants to merge 11 commits into
cogentcore:v25from
oliverbestmann:v27
Open

Various fixes against the v25 branch.#13
oliverbestmann wants to merge 11 commits into
cogentcore:v25from
oliverbestmann:v27

Conversation

@oliverbestmann
Copy link
Copy Markdown

@oliverbestmann oliverbestmann commented Nov 7, 2025

I can split the PR up into multiple small onces, but as the v25 PR is already one single big PR, i just went with another big one ;)

  • Update to go 24 in go.mod file
  • Go against wgpu 27 (current version)
  • Adjust js api to match the non-js api in multiple places
  • Fix runtime/cgo: misuse of an invalid Handle #6 by not using pointers to cgo.Handle
  • Expose wgpu DesiredMaximumFrameLatency in SurfaceConfiguration
  • Use an optimized implementation for Queue.WriteBuffer and Queue.WriteTexture
  • Handle missing VertexFormat in VertexFormat.Size()
  • Implement missing methods on Texture for js.

… that can be passed as an unsafe.Pointer or as a void* in c.
@oliverbestmann
Copy link
Copy Markdown
Author

Fixed the handle issue by introducing our own handle type that can be legally converted into an unsafe.Pointer.

@oliverbestmann
Copy link
Copy Markdown
Author

Note: i've now forked the repository and added more things on the main branch, that you might not want to have upstream. This includes improved garbage collection of wgpu resources, makeing calls to Release unecessary, as well as removing goal and moving the prebuild libraries into branches, as wgpu v27 is above the go get limit of 512mb.

@mreinstein
Copy link
Copy Markdown

It would be really nice to hear from the maintainers, if there are plans to either merge these improvements or cherry pick certain things out of it.

@rcoreilly
Copy link
Copy Markdown
Member

@kkoreilly and I will have a chance to discuss this soon and will post an update within a couple of weeks hopefully!

Copy link
Copy Markdown
Member

@kkoreilly kkoreilly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oliverbestmann Thanks for these changes! Apologies for the very delayed response, I've been quite busy.

Overall things look good, I just have a couple comments below.

Comment thread wgpu/queue_js.go
(queue, buf, offset, addr, x, n) => {
const mem = wasm.instance.exports.mem.buffer;
const data = new Uint8ClampedArray(mem, addr, n);
return queue.writeBuffer(buf, offset, data, x, n);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why this change is necessary as opposed to the previous jsx.BytesToJS approach. Does this solve the occasional crashing that occurred previously, or is there another motivation?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance iirc. It was precisely jumping to is and back a billion of times. Also yes, it crashed here previously, that got better now. One more thing: As you only take the uintptr of the Adress, the gc can theoretically dealloc the slice before it is used at the end of the function.

Comment thread go.mod
)

require (
cogentcore.org/core v0.3.12 // indirect
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is core now an indirect dependency?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I got rid of that too. I would need to check for what that is used ;)

@kkoreilly
Copy link
Copy Markdown
Member

@oliverbestmann The changes on your main branch look good overall, and if you are interested in filing separate PRs for them, that would be awesome (no worries if not).

For the binary size limit, I wonder whether it might be easier to use different modules within the same branch rather than in separate branches? You could have separate go.mod files in each lib/android etc directory, right?

Could you briefly explain how you implemented the garbage collection and whether there are any major negative side effects of that?

Thanks for all of your contributions, and apologies again for our unresponsiveness, I've been very busy but should hopefully have more time to respond going forward.

@oliverbestmann
Copy link
Copy Markdown
Author

Regarding the binary size limit: I am not sure if I tried that. I think the size limit is per shallow clone of the repository, not the module subdirectory.

Garbage collection: it is really just registering finalizers like here: https://github.com/oliverbestmann/webgpu/blob/main/wgpu/wgpu.go#L122 Special care needs to be taken when addRef is used, see a few lines above. The release method was made idempotent, so it can be called multiple times without crashing: https://github.com/oliverbestmann/webgpu/blob/main/wgpu/gen_types.go#L266
I did not experience any negative sideeffects.

I am currently also very busy due to $work and family, so I probably won't create multiple pull requests. I am sorry!

@rcoreilly
Copy link
Copy Markdown
Member

One consideration for timing of update of underlying libraries, assuming there remains some kind of cost to updating the libraries more frequently (hopefully @kkoreilly 's proposed solution might avoid that??) is the possible resolution of this issue: gfx-rs/wgpu#8119 -- this would probably be the single biggest issue fix for my usage.

@kkoreilly
Copy link
Copy Markdown
Member

@rcoreilly What is the timeline on gfx-rs/wgpu#8119? If it will be resolved soon, we can wait until then to finish this up. Otherwise we can work on merging this sooner (and I will experiment with the different approach for the binary libraries).

@rcoreilly
Copy link
Copy Markdown
Member

@kkoreilly IDK about the timing -- no evidence of progress or stated focus on this, so presumably not too soon. I think it would make sense for you to experiment with the approach for the binaries at this point.

@oliverbestmann
Copy link
Copy Markdown
Author

https://github.com/oliverbestmann/webgpu is now updated to 25.0.4.0

@oliverbestmann
Copy link
Copy Markdown
Author

https://github.com/oliverbestmann/webgpu is now updated to 25.0.4.0

Lets rephrase that, I've updated to wgpu-native v29.

@rcoreilly
Copy link
Copy Markdown
Member

@oliverbestmann I just tried to see if I could make a PR from your current main branch, and github says it is not possible because they have "entirely different commit histories".

In any case, it seems like you are continuing to actively update things, and I think at this point it would probably make sense for this repo to just point to yours as the definitive version, rather than trying to keep two copies in sync. @kkoreilly does this sound reasonable to you?

I will experiment with using yours now in cogentcore, and will probably file a PR for using glfw 3.4 -- would you be OK updating to that?

@oliverbestmann
Copy link
Copy Markdown
Author

Yea of course. I can probably do the glfw update myself later today or tomorrow too. Otherwise feel free to report whatever issue you might have, I am happy to fix those asap.

@oliverbestmann
Copy link
Copy Markdown
Author

I've updated to 3.4. There is a small issue if you want to use both x11 and wayland without build tags.
See go-gl/glfw#420

@rcoreilly
Copy link
Copy Markdown
Member

@oliverbestmann you don't have issues or discussions enabled on your repo page!

I am getting a nearly-100% reliable crash on Mac (Macbook Pro 2023, M3) in running the examples, which first appears in v1.30.0 (a254e95) when you updated to v29.0.0.0 Very occasionally it will run, but mostly it crashes, with the stack shown below.

If I use 337844f, examples run reliably.

This looks like a clear upstream, mac-specific issue. Not specific to glfw 3.4, but it may interact with glfw generically somehow -- seems like some kind of timing issue given the randomness of the crash. I'll look into the upstream system to see if I can reproduce anything there.

thread '<unnamed>' (21117468) panicked at src/lib.rs:4238:40:
invalid texture
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

thread '<unnamed>' (21117468) panicked at library/core/src/panicking.rs:225:5:
panic in a function that cannot unwind
stack backtrace:
   0:        0x1048423c0 - <std::sys::backtrace::BacktraceLock::print::DisplayBacktrace as core::fmt::Display>::fmt::hfd528c9d14c1136f
   1:        0x10486cf4c - core::fmt::write::h2ddadfca81aeb692
   2:        0x10480534c - std::io::Write::write_fmt::hc70e63382e9f29d2
   3:        0x104814f3c - std::sys::backtrace::BacktraceLock::print::hea663f6050de4073
   4:        0x10481b268 - std::panicking::default_hook::{{closure}}::hbcaf474e707f3b7d
   5:        0x10481b11c - std::panicking::default_hook::hff9145c2c6e89cdf
   6:        0x10481ba60 - std::panicking::panic_with_hook::he6431e171a67d1a5
   7:        0x10481b720 - std::panicking::panic_handler::{{closure}}::h42f8d27eb97efb5c
   8:        0x104815048 - std::sys::backtrace::__rust_end_short_backtrace::h0de35f7407a5e0d8
   9:        0x1047fbacc - __rustc[eb8946e36839644a]::rust_begin_unwind
  10:        0x1048b0b1c - core::panicking::panic_nounwind_fmt::h3db3f511675e5bd7
  11:        0x1048b0a1c - core::panicking::panic_nounwind::h50f882120c826cee
  12:        0x1048b0c20 - core::panicking::panic_cannot_unwind::h396781a6699a759f
  13:        0x10440a1d4 - _wgpuTextureCreateView
  14:        0x1043be610 - __cgo_0868a2725542_Cfunc_gowebgpu_texture_create_view
thread caused non-unwinding panic. aborting.
SIGABRT: abort
PC=0x18a1885e8 m=0 sigcode=0
signal arrived during cgo execution

goroutine 1 gp=0x140000021c0 m=0 mp=0x104c31aa0 [syscall, locked to thread]:
runtime.cgocall(0x1043be5cc, 0x1400007bc48)
	/opt/homebrew/Cellar/go/1.25.6/libexec/src/runtime/cgocall.go:167 +0x44 fp=0x1400007bc00 sp=0x1400007bbc0 pc=0x104347074
github.com/oliverbestmann/webgpu/wgpu._Cfunc_gowebgpu_texture_create_view(0x0, 0x0, 0x85b4adb40, 0x140000b8005)
	_cgo_gotypes.go:1768 +0x30 fp=0x1400007bc40 sp=0x1400007bc00 pc=0x10439b4a0
github.com/oliverbestmann/webgpu/wgpu.(*Texture).TryCreateView.func2(...)
	/Users/oreilly/go/src/github.com/oliverbestmann/webgpu/wgpu/texture.go:64
github.com/oliverbestmann/webgpu/wgpu.(*Texture).TryCreateView(0x14000112010, 0x100000104349280?)
	/Users/oreilly/go/src/github.com/oliverbestmann/webgpu/wgpu/texture.go:64 +0x1e8 fp=0x1400007bd20 sp=0x1400007bc40 pc=0x1043a31c8
main.(*State).Render(0x14000094080)
	/Users/oreilly/go/src/github.com/oliverbestmann/webgpu/examples/triangle/main.go:147 +0x4c fp=0x1400007be70 sp=0x1400007bd20 pc=0x1043aa21c
main.main()
	/Users/oreilly/go/src/github.com/oliverbestmann/webgpu/examples/triangle/glfw.go:49 +0x180 fp=0x1400007bf40 sp=0x1400007be70 pc=0x1043a9c90
runtime.main()

@oliverbestmann
Copy link
Copy Markdown
Author

Enabled the issues tab on the repo.
I can reproduce the issue on MacOS. It looks like it is this issue: gfx-rs/wgpu#4779
If I continue if the returned nextTexture is not valid, the app works but loops very quickly.

	fmt.Println("Next Texture is valid:", nextTexture.IsValid())
	if !nextTexture.IsValid() {
		return nil
	}

I'll see what I can do, maybe I can add a workaround.

@oliverbestmann
Copy link
Copy Markdown
Author

Done. I now correctly expose the status of the returned SurfaceTexture. You now need to handle this status and, for example, skip rendering if the surface is occluded. See the cube example for reference.

@rcoreilly
Copy link
Copy Markdown
Member

Thanks for the fix -- I was trying something similar in a fork but hoping to somehow avoid having every single program anyone ever writes to include that logic.. I don't see how to encapsulate it anywhere sensible though.

Here's the actual issue with links to all associated issues and PRs: gfx-rs/wgpu#9430

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants